Feature/extend logger to user activity#42
Merged
Conversation
- Created UserLogDTO, UserEventType, UserLog, UserLogMapper, UserLogRepository, UserLogService by mirroring the existing AuditLog files - Renamed all AuditLog files to VisaLog to differentiate from UserLog - Updated UserService to record UserLog events Fix: - Renamed all usages of AuditLog in all files to VisaLog - Fixed tests failing because of AuditLog mentions or not taking UserService changes into consideration.
- UserLogMapper - UserLogService - VisaLogMapper - VisaLogService
Closed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughSplit the generic audit subsystem into two domain-specific paths: user logs (UserLog/UserEventType) and visa logs (VisaLog/VisaEventType). Adds entities, DTOs, mappers, repositories, services, controller endpoints, template updates, tests, and an AccessDeniedException handler. Mutating APIs now accept an explicit actor userId. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as UserViewController
participant Service as UserService
participant LogService as UserLogService
participant Mapper as UserLogMapper
participant Repo as UserLogRepository
participant DB as Database
Client->>Controller: POST /profile/edit/{userId}/authorization
Controller->>Service: updateUserAuthorization(actorUserId, targetUserId, newAuth)
Service->>Service: validate & persist user (UserRepository)
Service->>LogService: createUserLog(actorUserId, targetUserId, AUTHORIZATION_CHANGED, desc)
LogService->>Mapper: toEntity(actorUserId, targetUserId, UserEventType, desc)
Mapper-->>LogService: UserLog entity
LogService->>Repo: save(UserLog)
Repo->>DB: INSERT user_log
DB-->>Repo: OK
Repo-->>LogService: saved UserLog
LogService-->>Service: completed
Service-->>Controller: redirect/view
Controller-->>Client: HTTP 302 / updated page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java (1)
161-170:⚠️ Potential issue | 🟠 MajorSysadmin dashboard omits user-activity logs.
sysAdminDashboardexposes onlyvisaLogsin the model. Given this PR introducesUserLogServicespecifically for user-activity auditing, consider also injectingUserLogServiceand addingmodel.addAttribute("userLogs", userLogService.findAll())(and surfacing them indashboard/sysadmin.html). Otherwise, user audit logs are written but never viewable, making the new infrastructure effectively dark. (Downstream impact also noted indashboard/sysadmin.html.)♻️ Sketch of the change
private final VisaService visaService; private final UserService userService; private final VisaLogService visaLogService; + private final UserLogService userLogService; public UserViewController(VisaService visaService, UserService userService, - VisaLogService visaLogService) { + VisaLogService visaLogService, + UserLogService userLogService) { this.visaService = visaService; this.userService = userService; this.visaLogService = visaLogService; + this.userLogService = userLogService; } ... model.addAttribute("visaLogs", visaLogService.findAll()); + model.addAttribute("userLogs", userLogService.findAll()); return "dashboard/sysadmin";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java` around lines 161 - 170, The sysAdminDashboard method currently only exposes visaLogs; inject the new UserLogService into the controller (e.g., add a constructor or field for UserLogService) and in sysAdminDashboard(...) call model.addAttribute("userLogs", userLogService.findAll()) so user activity audit records are available to the view (also ensure dashboard/sysadmin.html is updated to render the userLogs). Locate the sysAdminDashboard method and the controller class to add the UserLogService dependency and the model attribute.src/main/resources/templates/dashboard/sysadmin.html (1)
188-229:⚠️ Potential issue | 🟠 MajorUser activity logs are not surfaced on the sysadmin dashboard.
The PR splits the monolithic audit log into
UserLogandVisaLog, but the sysadmin dashboard now only displaysvisaLogs. User-activity events written throughUserLogService(created/updated/deleted users, authorization changes) have no UI, which partially defeats the "extend logger to user activity" goal. Consider adding a sibling "User Log" section iterating over auserLogsattribute populated fromuserLogService.findAll()inUserViewController.sysAdminDashboard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/dashboard/sysadmin.html` around lines 188 - 229, The dashboard only renders visaLogs; add a parallel "User Log" section and wire the controller to supply userLogs so user activity appears. In UserViewController.sysAdminDashboard call UserLogService.findAll() (or equivalent method) and add the returned list to the model as "userLogs"; then update the sysadmin.html template to include a new container similar to the Visa Log block that iterates th:each="log : ${userLogs}" and renders fields from UserLog (e.g., log.timeStamp(), log.userId(), log.eventType(), log.description()) to match the existing table structure. Ensure the new section uses the same empty-state logic and pluralized row-count expression as the Visa Log.
🧹 Nitpick comments (5)
src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.java (1)
56-56: Tighten this audit-log assertion.
isNotEmpty()would still pass if the service wrote the wrong visa event, user, or case. Since this test covers the new visa-audit path, assert theCREATEDlog belongs to the saved visa and applicant.Proposed test assertion
- assertThat(visaLogService.findAll()).isNotEmpty(); + var logs = visaLogService.findAll(); + Visa savedVisa = savedVisas.get(0); + assertThat(logs).anyMatch(log -> + savedVisa.getId().equals(log.visaCaseId()) && + user.getId().equals(log.userId()) && + log.visaEventType() == VisaEventType.CREATED + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.java` at line 56, Replace the loose isNotEmpty() assertion with one that verifies the audit log contains a CREATED entry for the visa we saved and the applicant who acted: query visaLogService.findAll() and assert it containsAnyMatch/log entry where log.getEvent() == CREATED (or Event.CREATED), log.getSubjectId().equals(savedVisa.getId()) and log.getActorId().equals(savedApplicant.getId()) (or equivalent getter names used in the test), so the test ensures the correct event, subject (visa id) and actor (applicant/user id) are recorded.src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java (1)
58-111: Consider asserting audit-log side effects.Since the whole point of this PR is to record user activity, these integration tests for
updateUser,updateUserAuthorization, anddeleteUserare an ideal place to assert that a correspondingUserLogrow is persisted (e.g., autowireUserLogRepositoryand check count +userEventType/actorUserId/targetUserId). Right now the audit side effect is completely untested at the integration layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java` around lines 58 - 111, Add assertions that the audit UserLog row is persisted for each integration test: autowire UserLogRepository into UserServiceIntegrationTest, record the initial count (or query for logs for targetUserId) before calling userService methods in updateUser_shouldUpdateUserFields_WhenDataIsValid, updateUserAuthorization_shouldChangeAuthorization_WhenRequestedBySysAdmin, and deleteUser_shouldRemoveUser_WhenRequestedBySysAdmin, then after the Act assert the repository contains one new UserLog with the expected userEventType (e.g., UPDATE_PROFILE / UPDATE_AUTHORIZATION / DELETE_USER), actorUserId equals the acting user's id (user.getId() or sysAdmin.getId()), and targetUserId equals the affected user's id (user.getId() or targetUser.getId()); prefer querying the latest log for the target user and assert its fields rather than relying solely on total count.src/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.java (1)
66-72: Consider adding a dedicatedCOMMENT_ADDEDevent type toVisaEventTypeif comment events warrant distinct audit logging.Using
VisaEventType.UPDATEDfor comment creation conflates it with actual visa-status updates, making the audit log less precise. TheVisaEventTypeenum currently has no comment-specific event type (only CREATED, ASSIGNED, UPDATED, DELETED, GRANTED, REJECTED), so addingCOMMENT_ADDEDwould require enum changes. If comment logging should be tracked separately from visa updates, this refactoring is worth considering; otherwise, the current approach usingUPDATEDis acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.java` around lines 66 - 72, Change the audit event for comment creation to a dedicated event type: add COMMENT_ADDED to the VisaEventType enum and update the call in CommentService where visaLogService.createVisaLog(...) is invoked (the block that currently passes VisaEventType.UPDATED with message "Comment added by ...") to use VisaEventType.COMMENT_ADDED instead; ensure any switch/case or serialization that handles VisaEventType is updated to recognize COMMENT_ADDED and add a migration or tests if enum persistence is used.src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.java (1)
12-33: Same@Transactionalsuggestion asVisaLogService.Apply the same explicit transactional boundaries here (
@TransactionaloncreateUserLog,@Transactional(readOnly = true)onfindAll) for consistency and clarity across the two audit services.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.java` around lines 12 - 33, Add explicit transactional boundaries to UserLogService by annotating createUserLog with `@Transactional` and findAll with `@Transactional`(readOnly = true); modify the UserLogService class to import and apply these annotations to the corresponding methods (createUserLog and findAll) to match the VisaLogService pattern and ensure consistent transaction semantics.src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java (1)
12-33: Consider adding explicit@Transactionalboundaries.
createVisaLogperforms a write andfindAllis read-only, yet the service has no transactional annotations. Relying onSimpleJpaRepository's internal transactions works, but explicit boundaries are clearer and ensure that when callers don't supply a transaction, behavior is predictable (andfindAllbenefits fromreadOnly=true).♻️ Proposed refactor
`@Service` public class VisaLogService { @@ - public void createVisaLog(Long userId, Long visaCaseId, VisaEventType visaEventType, String description) { + `@Transactional` + public void createVisaLog(Long userId, Long visaCaseId, VisaEventType visaEventType, String description) { VisaLog visaLog = visaLogMapper.toEntity(userId, visaCaseId, visaEventType, description); visaLogRepository.save(visaLog); } - public List<VisaLogDTO> findAll() { + `@Transactional`(readOnly = true) + public List<VisaLogDTO> findAll() { return visaLogRepository.findAll()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java` around lines 12 - 33, Add explicit transactional boundaries to VisaLogService: annotate createVisaLog with `@Transactional` (default readOnly=false) and annotate findAll with `@Transactional`(readOnly = true); ensure the org.springframework.transaction.annotation.Transactional import is added and put the annotations on the methods (or annotate the class with `@Transactional` and override readOnly=true on findAll) so writes go through an explicit transaction and reads use a read-only transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/visacasemanagementsystem/audit/entity/UserLog.java`:
- Around line 32-41: The Spotless formatting check fails because the `@NotNull`
annotations for the fields actorUserId and targetUserId in the UserLog entity
are on their own lines; update the declarations in class UserLog so the
annotations are inline with the fields (e.g., change from "@NotNull" on a
separate line to "@NotNull private Long actorUserId;" and similarly for
targetUserId), matching the style used in VisaLog; you can run mvn
spotless:apply afterwards to verify formatting.
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 109-121: The updateUserAuthorization method must validate the
incoming newAuth before modifying or saving the entity and before creating the
audit log; check that newAuth is not null (and optionally a supported enum
value) at the start of updateUserAuthorization, throw a suitable exception
(e.g., IllegalArgumentException or a validation exception) if invalid, and only
then call user.setUserAuthorization(newAuth), userRepository.save(user), and
userLogService.createUserLog (so the savedUser and log never record a null or
invalid authorization).
- Around line 69-82: The catch block around userRepository.save currently also
wraps the subsequent userLogService.createUserLog call so any audit failures are
misreported as duplicate-email; update UserService to only catch
DataIntegrityViolationException from the persistence/save step
(userRepository.save) and let userLogService.createUserLog run outside that try
or be handled by its own try/catch that logs audit errors without rethrowing as
IllegalArgumentException. Specifically, isolate the save into its own try that
throws the "A user with this email already exists" IllegalArgumentException on
DataIntegrityViolationException, then call
userLogService.createUserLog(savedUser.getId(), savedUser.getId(),
UserEventType.CREATED, ...) afterward (or catch/log audit exceptions separately)
and finally return userMapper.toDTO(savedUser).
---
Outside diff comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java`:
- Around line 161-170: The sysAdminDashboard method currently only exposes
visaLogs; inject the new UserLogService into the controller (e.g., add a
constructor or field for UserLogService) and in sysAdminDashboard(...) call
model.addAttribute("userLogs", userLogService.findAll()) so user activity audit
records are available to the view (also ensure dashboard/sysadmin.html is
updated to render the userLogs). Locate the sysAdminDashboard method and the
controller class to add the UserLogService dependency and the model attribute.
In `@src/main/resources/templates/dashboard/sysadmin.html`:
- Around line 188-229: The dashboard only renders visaLogs; add a parallel "User
Log" section and wire the controller to supply userLogs so user activity
appears. In UserViewController.sysAdminDashboard call UserLogService.findAll()
(or equivalent method) and add the returned list to the model as "userLogs";
then update the sysadmin.html template to include a new container similar to the
Visa Log block that iterates th:each="log : ${userLogs}" and renders fields from
UserLog (e.g., log.timeStamp(), log.userId(), log.eventType(),
log.description()) to match the existing table structure. Ensure the new section
uses the same empty-state logic and pluralized row-count expression as the Visa
Log.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.java`:
- Around line 12-33: Add explicit transactional boundaries to UserLogService by
annotating createUserLog with `@Transactional` and findAll with
`@Transactional`(readOnly = true); modify the UserLogService class to import and
apply these annotations to the corresponding methods (createUserLog and findAll)
to match the VisaLogService pattern and ensure consistent transaction semantics.
In
`@src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java`:
- Around line 12-33: Add explicit transactional boundaries to VisaLogService:
annotate createVisaLog with `@Transactional` (default readOnly=false) and annotate
findAll with `@Transactional`(readOnly = true); ensure the
org.springframework.transaction.annotation.Transactional import is added and put
the annotations on the methods (or annotate the class with `@Transactional` and
override readOnly=true on findAll) so writes go through an explicit transaction
and reads use a read-only transaction.
In
`@src/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.java`:
- Around line 66-72: Change the audit event for comment creation to a dedicated
event type: add COMMENT_ADDED to the VisaEventType enum and update the call in
CommentService where visaLogService.createVisaLog(...) is invoked (the block
that currently passes VisaEventType.UPDATED with message "Comment added by ...")
to use VisaEventType.COMMENT_ADDED instead; ensure any switch/case or
serialization that handles VisaEventType is updated to recognize COMMENT_ADDED
and add a migration or tests if enum persistence is used.
In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.java`:
- Around line 58-111: Add assertions that the audit UserLog row is persisted for
each integration test: autowire UserLogRepository into
UserServiceIntegrationTest, record the initial count (or query for logs for
targetUserId) before calling userService methods in
updateUser_shouldUpdateUserFields_WhenDataIsValid,
updateUserAuthorization_shouldChangeAuthorization_WhenRequestedBySysAdmin, and
deleteUser_shouldRemoveUser_WhenRequestedBySysAdmin, then after the Act assert
the repository contains one new UserLog with the expected userEventType (e.g.,
UPDATE_PROFILE / UPDATE_AUTHORIZATION / DELETE_USER), actorUserId equals the
acting user's id (user.getId() or sysAdmin.getId()), and targetUserId equals the
affected user's id (user.getId() or targetUser.getId()); prefer querying the
latest log for the target user and assert its fields rather than relying solely
on total count.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.java`:
- Line 56: Replace the loose isNotEmpty() assertion with one that verifies the
audit log contains a CREATED entry for the visa we saved and the applicant who
acted: query visaLogService.findAll() and assert it containsAnyMatch/log entry
where log.getEvent() == CREATED (or Event.CREATED),
log.getSubjectId().equals(savedVisa.getId()) and
log.getActorId().equals(savedApplicant.getId()) (or equivalent getter names used
in the test), so the test ensures the correct event, subject (visa id) and actor
(applicant/user id) are recorded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19c5de37-c089-4b51-bd50-441059bb9e0f
📒 Files selected for processing (28)
src/main/java/org/example/visacasemanagementsystem/audit/UserEventType.javasrc/main/java/org/example/visacasemanagementsystem/audit/VisaEventType.javasrc/main/java/org/example/visacasemanagementsystem/audit/dto/UserLogDTO.javasrc/main/java/org/example/visacasemanagementsystem/audit/dto/VisaLogDTO.javasrc/main/java/org/example/visacasemanagementsystem/audit/entity/UserLog.javasrc/main/java/org/example/visacasemanagementsystem/audit/entity/VisaLog.javasrc/main/java/org/example/visacasemanagementsystem/audit/mapper/AuditMapper.javasrc/main/java/org/example/visacasemanagementsystem/audit/mapper/UserLogMapper.javasrc/main/java/org/example/visacasemanagementsystem/audit/mapper/VisaLogMapper.javasrc/main/java/org/example/visacasemanagementsystem/audit/repository/UserLogRepository.javasrc/main/java/org/example/visacasemanagementsystem/audit/repository/VisaLogRepository.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/AuditService.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.javasrc/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.javasrc/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/java/org/example/visacasemanagementsystem/user/service/UserService.javasrc/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.javasrc/main/resources/templates/dashboard/sysadmin.htmlsrc/test/java/org/example/visacasemanagementsystem/audit/mapper/UserLogMapperTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/mapper/VisaLogMapperTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/service/UserLogServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/service/VisaLogServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/user/service/UserServiceIntegrationTest.javasrc/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.java
💤 Files with no reviewable changes (2)
- src/main/java/org/example/visacasemanagementsystem/audit/mapper/AuditMapper.java
- src/main/java/org/example/visacasemanagementsystem/audit/service/AuditService.java
…ing to CodeRabbit suggestion
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java (1)
59-65: Consider asserting no audit log is emitted on failed operations.These failure-path tests verify exceptions, but they don’t protect against accidental
userLogService.createUserLog(...)calls before validation, lookup, or persistence succeeds.🧪 Suggested test hardening
assertThatThrownBy(() -> userService.createUser(dto)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("email already exists"); +verifyNoInteractions(userLogService);Apply the same pattern to the other failure-path tests where the operation should not create an audit record.
Also applies to: 82-84, 128-130, 148-150, 191-193, 241-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java` around lines 59 - 65, The tests currently assert exceptions and repository non-interaction but miss asserting no audit log was written; update each failure-path test (e.g., the createUser negative-case that calls userService.createUser(dto)) to also verify that userLogService was not invoked by adding a Mockito assertion such as verifyNoInteractions(userLogService) or verify(userLogService, never()).method(any()) after the exception assertion (mirror this change for other failure-path tests that validate inputs or early failures to ensure userLogService.createUserLog(...) is never called).src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java (1)
159-169: DuplicatedisSysAdmincomputation — already computed inviewProfile.The sysadmin-detection logic (streaming authorities and matching
ROLE_SYSADMIN) appears in bothviewProfile(lines 91-92) andaddAuthorizationFormAttributes. Consider hoisting to a small helper onUserPrincipal(e.g.,principal.isSysAdmin()) or a private static method here to keep the check in one place.♻️ Suggested refactor
- private void addAuthorizationFormAttributes(Model model, UserPrincipal principal, Long userId) { - boolean isOwnProfile = principal.getUserId().equals(userId); - boolean isSysAdmin = principal.getAuthorities().stream() - .anyMatch(a -> Objects.equals(a.getAuthority(), "ROLE_SYSADMIN")); - model.addAttribute("canChangeAuthorization", isSysAdmin && !isOwnProfile); - model.addAttribute("authorizations", UserAuthorization.values()); - } + private static boolean isSysAdmin(UserPrincipal principal) { + return principal.getAuthorities().stream() + .anyMatch(a -> Objects.equals(a.getAuthority(), "ROLE_SYSADMIN")); + } + + private void addAuthorizationFormAttributes(Model model, UserPrincipal principal, Long userId) { + boolean isOwnProfile = principal.getUserId().equals(userId); + model.addAttribute("canChangeAuthorization", isSysAdmin(principal) && !isOwnProfile); + model.addAttribute("authorizations", UserAuthorization.values()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java` around lines 159 - 169, Duplicate sysadmin check appears in UserViewController.viewProfile and UserViewController.addAuthorizationFormAttributes; hoist it to a single helper to avoid duplication. Add a boolean helper like UserPrincipal.isSysAdmin() (preferred) or a private static method in UserViewController that encapsulates principal.getAuthorities().stream().anyMatch(a -> Objects.equals(a.getAuthority(), "ROLE_SYSADMIN")), then replace the inline checks in both viewProfile and addAuthorizationFormAttributes with calls to that helper (update references to isSysAdmin variable usage accordingly).src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java (1)
404-422: Consider also asserting theuserLogsmodel attribute on the sysadmin dashboard test.
sysAdminDashboardnow populates bothvisaLogsanduserLogs(controller lines 211-212), and the template iteratesuserLogs. The updated test at lines 366-382 only stubsvisaLogService.findAll()and assertsvisaLogsexists. Adding auserLogService.findAll()stub and amodel().attributeExists("userLogs")assertion would keep coverage aligned with the template's requirements and prevent a silent regression ifuserLogsis ever dropped from the model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java` around lines 404 - 422, Update the sysAdmin dashboard test in UserViewControllerTest to also stub the userLogService and assert the userLogs model attribute: add a when(userLogService.findAll()).thenReturn(...) alongside the existing when(visaLogService.findAll()) stub, and add .andExpect(model().attributeExists("userLogs")) to the mockMvc assertion chain in the test method that exercises sysAdminDashboard (look for the test method that calls "/admin" or is named sysAdminDashboard); also verify any interactions with userLogService if other tests follow that pattern.src/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.java (1)
32-40: Duplicated handler body — consider delegating to a shared helper.
handleAccessDeniedExceptionis byte-identical tohandleUnauthorizedException(same log message, same two model attributes, same view). Minor DRY concern; easy to let them drift over time (e.g., adding the pendingdashboardUrlTODO to only one).♻️ Suggested refactor
- `@ResponseStatus`(value = HttpStatus.FORBIDDEN) - `@ExceptionHandler`(UnauthorizedException.class) - public String handleUnauthorizedException(UnauthorizedException exception, Model model) { - log.error("Access denied: {}", exception.getMessage()); - model.addAttribute("errorMessage","You do not have permission to perform this action."); - model.addAttribute("errorTitle", "⚠️Access Denied."); - // TODO: ... - return "error/error"; - } - - // Handles method-level `@PreAuthorize` denials which throw AccessDeniedException - `@ResponseStatus`(value = HttpStatus.FORBIDDEN) - `@ExceptionHandler`(AccessDeniedException.class) - public String handleAccessDeniedException(AccessDeniedException exception, Model model) { - log.error("Access denied: {}", exception.getMessage()); - model.addAttribute("errorMessage", "You do not have permission to perform this action."); - model.addAttribute("errorTitle", "⚠️Access Denied."); - return "error/error"; - } + `@ResponseStatus`(value = HttpStatus.FORBIDDEN) + `@ExceptionHandler`({UnauthorizedException.class, AccessDeniedException.class}) + public String handleForbidden(Exception exception, Model model) { + log.error("Access denied: {}", exception.getMessage()); + model.addAttribute("errorMessage", "You do not have permission to perform this action."); + model.addAttribute("errorTitle", "⚠️Access Denied."); + return "error/error"; + }Worth confirming this is reachable by
@ControllerAdviceat all: Spring Security'sAccessDeniedExceptionthrown by@PreAuthorizeviaAuthorizationManagerBeforeMethodInterceptoris normally handled by the Spring Security filter chain'sExceptionTranslationFilter/AccessDeniedHandlerbefore@ExceptionHandleradvice gets a chance, especially for filter-level denials. For method-security denials thrown inside the controller invocation,@ExceptionHandlerdoes pick them up. Please confirm with a manual test that hitting a@PreAuthorize-denied endpoint (e.g.,/dashboard/sysadminas a USER) actually renderserror/errorwitherrorTitle = "⚠️Access Denied."rather than Spring Security's default 403 page — the existinguserListView_AsNonSysAdmin_ShouldReturnForbiddentest only assertsstatus().isForbidden()and won't catch this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.java` around lines 32 - 40, Both handleAccessDeniedException and handleUnauthorizedException in GlobalExceptionHandler have identical bodies; extract the shared logic into a single private helper (e.g., a method like renderErrorView(Model model, Exception e, String title, String message)) and have both exception handlers call that helper to set the log message, model attributes ("errorMessage", "errorTitle") and return the "error/error" view; update handleAccessDeniedException and handleUnauthorizedException to delegate to this helper so future changes (like adding dashboardUrl) are made in one place and avoid drift, and after refactor run the manual test described to confirm the ControllerAdvice handler is actually reached for method-level AccessDeniedException.src/main/resources/templates/dashboard/sysadmin.html (1)
192-272: Null-safety onvisaLogs/userLogslookups.
${visaLogs.isEmpty()},${!visaLogs.isEmpty()},${#lists.size(visaLogs) ...}(and theuserLogsequivalents) will NPE if either attribute is ever missing/null in the model. TodaysysAdminDashboardalways sets both, so this is latent rather than broken — but it's cheap defensive rendering in case an error path or future caller omits them.♻️ Suggested hardening
- <div th:if="${visaLogs.isEmpty()}" class="empty-state"> + <div th:if="${`#lists.isEmpty`(visaLogs)}" class="empty-state"> No visa events recorded yet. </div> - <table th:if="${!visaLogs.isEmpty()}"> + <table th:if="${!#lists.isEmpty(visaLogs)}">(Same pattern for
userLogs.)#lists.isEmpty(...)/#lists.size(...)handle null gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/dashboard/sysadmin.html` around lines 192 - 272, The template currently calls visaLogs.isEmpty() and userLogs.isEmpty() which can NPE if the model omits those attributes; replace those checks with the null-safe Thymeleaf list utilities: use `#lists.isEmpty`(visaLogs) wherever visaLogs.isEmpty() or !visaLogs.isEmpty() is used (e.g. the th:if checks for the empty-state and the table) and do the same for userLogs (use `#lists.isEmpty`(userLogs) and negated form for showing the table); leave the existing `#lists.size`(visaLogs) / `#lists.size`(userLogs) usage as-is because those are already null-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java`:
- Around line 127-138: When rebuilding the UserDTO in UserViewController's catch
for IllegalArgumentException, guard against userService.findById(userId)
returning empty by falling back to any existing user authorization already
present in the model or request instead of null; i.e., obtain currentAuth via
userService.findById(userId).map(UserDTO::userAuthorization).orElseGet(() -> {
UserDTO existing = (UserDTO) model.getAttribute("user"); return existing != null
? existing.userAuthorization() : null; }) and then use that currentAuth when
constructing the new UserDTO and calling addAuthorizationFormAttributes so the
authorization dropdown never becomes null/empty.
- Around line 144-157: The updateAuthorization method currently lets
service-thrown IllegalArgumentException bubble to the global handler (rendering
the generic error page); catch IllegalArgumentException around the call to
userService.updateUserAuthorization in updateAuthorization and handle it the
same way updateProfile does — add a user-friendly error message (from the
exception) to RedirectAttributes or the model and return/redirect back to the
profile edit page (e.g., "redirect:/profile/edit/{userId}" or the edit view) so
validation/service errors show inline; keep the existing UnauthorizedException
self-change guard and still rethrow it unchanged.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.java`:
- Around line 32-40: Both handleAccessDeniedException and
handleUnauthorizedException in GlobalExceptionHandler have identical bodies;
extract the shared logic into a single private helper (e.g., a method like
renderErrorView(Model model, Exception e, String title, String message)) and
have both exception handlers call that helper to set the log message, model
attributes ("errorMessage", "errorTitle") and return the "error/error" view;
update handleAccessDeniedException and handleUnauthorizedException to delegate
to this helper so future changes (like adding dashboardUrl) are made in one
place and avoid drift, and after refactor run the manual test described to
confirm the ControllerAdvice handler is actually reached for method-level
AccessDeniedException.
In
`@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java`:
- Around line 159-169: Duplicate sysadmin check appears in
UserViewController.viewProfile and
UserViewController.addAuthorizationFormAttributes; hoist it to a single helper
to avoid duplication. Add a boolean helper like UserPrincipal.isSysAdmin()
(preferred) or a private static method in UserViewController that encapsulates
principal.getAuthorities().stream().anyMatch(a ->
Objects.equals(a.getAuthority(), "ROLE_SYSADMIN")), then replace the inline
checks in both viewProfile and addAuthorizationFormAttributes with calls to that
helper (update references to isSysAdmin variable usage accordingly).
In `@src/main/resources/templates/dashboard/sysadmin.html`:
- Around line 192-272: The template currently calls visaLogs.isEmpty() and
userLogs.isEmpty() which can NPE if the model omits those attributes; replace
those checks with the null-safe Thymeleaf list utilities: use
`#lists.isEmpty`(visaLogs) wherever visaLogs.isEmpty() or !visaLogs.isEmpty() is
used (e.g. the th:if checks for the empty-state and the table) and do the same
for userLogs (use `#lists.isEmpty`(userLogs) and negated form for showing the
table); leave the existing `#lists.size`(visaLogs) / `#lists.size`(userLogs) usage
as-is because those are already null-safe.
In
`@src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java`:
- Around line 404-422: Update the sysAdmin dashboard test in
UserViewControllerTest to also stub the userLogService and assert the userLogs
model attribute: add a when(userLogService.findAll()).thenReturn(...) alongside
the existing when(visaLogService.findAll()) stub, and add
.andExpect(model().attributeExists("userLogs")) to the mockMvc assertion chain
in the test method that exercises sysAdminDashboard (look for the test method
that calls "/admin" or is named sysAdminDashboard); also verify any interactions
with userLogService if other tests follow that pattern.
In
`@src/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java`:
- Around line 59-65: The tests currently assert exceptions and repository
non-interaction but miss asserting no audit log was written; update each
failure-path test (e.g., the createUser negative-case that calls
userService.createUser(dto)) to also verify that userLogService was not invoked
by adding a Mockito assertion such as verifyNoInteractions(userLogService) or
verify(userLogService, never()).method(any()) after the exception assertion
(mirror this change for other failure-path tests that validate inputs or early
failures to ensure userLogService.createUserLog(...) is never called).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89d8793c-f38e-4ea6-9de3-7e0484edbc30
📒 Files selected for processing (8)
src/main/java/org/example/visacasemanagementsystem/audit/entity/UserLog.javasrc/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.javasrc/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/java/org/example/visacasemanagementsystem/user/service/UserService.javasrc/main/resources/templates/dashboard/sysadmin.htmlsrc/main/resources/templates/profile/edit.htmlsrc/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/user/service/UserServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/example/visacasemanagementsystem/audit/entity/UserLog.java
- src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java
…r form and page redirection on AccessDenied error
Closed
…ivity # Conflicts: # src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java # src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.java
This was referenced Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes